-
Notifications
You must be signed in to change notification settings - Fork 1k
use generated types in c3 #8886
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-wrangler-8886You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/8886/npm-package-wrangler-8886Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-wrangler-8886 dev path/to/script.jsAdditional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-cloudflare-workers-bindings-extension-8886 -O ./cloudflare-workers-bindings-extension.0.0.0-v5e3b99075.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v5e3b99075.vsixcreate-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-create-cloudflare-8886 --no-auto-update@cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-cloudflare-kv-asset-handler-8886miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-miniflare-8886@cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-cloudflare-pages-shared-8886@cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-cloudflare-unenv-preset-8886@cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-cloudflare-vite-plugin-8886@cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-cloudflare-vitest-pool-workers-8886@cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-cloudflare-workers-editor-shared-8886@cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-cloudflare-workers-shared-8886@cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14406763126/npm-package-cloudflare-workflows-shared-8886Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
🦋 Changeset detectedLatest commit: c5ca33b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
7017389 to
9f47f3a
Compare
57357b0 to
dc30b81
Compare
|
These changes have been automatically backported to Wrangler v3 🎉 You can view the automatically updated PR at v3-maintenance...v3-maintenance-8886. Please check that PR for correctness, and make sure it's merged after this one. Thank you for helping us keep Wrangler v3 supported! |
|
A Wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-wrangler-8886Prereleases for other packages:
wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-cloudflare-workers-bindings-extension-8886 -O ./cloudflare-workers-bindings-extension.0.0.0-v9c97da27b.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-v9c97da27b.vsix
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-create-cloudflare-8886 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-cloudflare-kv-asset-handler-8886
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-miniflare-8886
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-cloudflare-pages-shared-8886
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-cloudflare-unenv-preset-8886
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-cloudflare-vite-plugin-8886
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-cloudflare-vitest-pool-workers-8886
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-cloudflare-workers-editor-shared-8886
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-cloudflare-workers-shared-8886
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/14903120110/npm-package-cloudflare-workflows-shared-8886Note that these links will no longer work once the GitHub Actions artifact expires. |
| }, | ||
| }, | ||
| }, | ||
| compatibilityFlags: ["nodejs_compat"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the flag was already there (in the wrangler configs), just not in the c3 templateConfig here. Its not great that they can be out of sync like this, but this sort of thing happens all over c3 and is probably beyond the scope of this PR to fix. (I'll make sure I don't add to the problem though...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at it when working on #9147
This flag is only used by pages (9147 updates the description).
So I think it can be removed from here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we usecompatibilityFlags in this PR here to make sure @types/node is installed. i can update the comment to reflect this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually to get around the issue of this getting out of sync, and to support remote templates, that bit should probably read the wrangler config instead - i'll update that.
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments.
Is there any reason why i.e. Next is not in this PR?
next + workers was in the pr, but i seem to have left out next on pages by accident - ac7026a (well everything is 'included' automatically, but there was a redundant file to delete) |
2c9506f to
4d5aab7
Compare
|
@emily-shen could you please rebase the PR and answer the comments? |
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added inline comments.
The PR contains quite a lot of changes.
I think it would help reviewing if the description of the PR lists what is changed in the PR - something to keep in mind for future PRs.
Thanks!
Ah great feedback, i've updated it now for posterity (and good habits!) |
| expect(hasEnvInterfacePre).toBe(true); | ||
|
|
||
| // Run the `cf-typegen` script to generate types for bindings in fixture | ||
| const buildTypesProc = spawnWithLogging( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its useful here to test wrangler types works, so instead this now tests the immediate output of c3 is as expected.
Co-authored-by: Victor Berchet <[email protected]>
7008d4d to
c5ca33b
Compare
vicb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
Thanks Emily!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting this file has broken the CI in Version Packages @emily-shen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repro:
npx create-cloudflare@beta --platform=pages --framework=next -- --yes
It is because we need to also remove the bit in c3.ts that is trying to copy it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We missed it in the PR CI jobs because next-on-pages was excluded from pnpm runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix in #9182
Fixes DEVX-1613
This PR updates c3 to use types generated by
wrangler typesinstead of installing@cloudflare/workers-types.worker-configuration.d.tsetc. from framework templates, as this now gets generated automatically.A few frameworks (remix and qwik) pin wrangler 3, which generate runtime types in a different way, so for simplicity we continue installing workers-types for those frameworks. This can be cleaned up in a followup if/when those frameworks upgrade to wrangler v4.
Testing for framework templates has also been updated to always check types based on the template config (ie verifyBuildCfTypes is no longer necessary), and checks the immediate output of c3 instead of the output after re-running typegen.